feat(outbound): require single-use provider authorization (B5) - #998
Merged
Conversation
testutil bundles the contract server, which imports outbound. Once outbound imports sendingpolicy (the provider seam requires the gate's token), any INTERNAL test file in outbound or sendingpolicy that calls testutil.TestDB closes an import cycle. Move db.go into internal/testutil/testdb, a leaf that depends only on identity and migrations, and keep testutil.TestDB / TestDBURL / OpenPreparedTestDB / TruncateAll as forwarding wrappers so every existing caller is unchanged. The contract server and its River test use the two newly exported helpers (Truncate, BaseTestDBURL) instead of package-private ones. The one internal sendingpolicy test imports testdb directly. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjfGxvXW6fNKWGFHuo68yX
Add ProviderSubmitter, the one path from a composed message to an SMTP transaction with SES. It requires a sendingpolicy.ProviderAuthorization for every call, proves the envelope is the authorized one, derives X-E2A-Provider-Attempt and X-SES-TENANT only from the token (and X-SES-CONFIGURATION-SET from configuration), strips every occurrence of those headers — mixed case and folded forms — from the composed MIME, redeems the token, and only then dials. Every early exit is I/O-free and leaves the token intact; a misconfigured relay is refused before redemption so a retry does not burn an ordinal. A definite permanent rejection is settled as such; an acceptance is settled with the provider's message id, which SettleProvider now binds to the attempt's feedback correlation exactly once (same id replays, a different id is refused). A settlement that fails after acceptance is reported on the result, never as a send error, so the caller retries settlement rather than resubmitting. The legacy tokenless Sender.SubmitOnce path stays for now; Task 7 migrates its callers and makes the relay's socket-opening methods package-private. Tests: zero-network on missing, mismatched, stale, and reused tokens (asserted against a socket counter); exact and single attempt/tenant headers with smuggled spellings removed and the body untouched; a physical retry redeems a distinct ordinal; provider id binding and its conflict rule. Removing the header strip, reordering redemption before envelope validation, or dropping the id binding each fail a test. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjfGxvXW6fNKWGFHuo68yX
Two review passes (correctness + adversarial) over the first cut. Every finding below has a named test, and removing each fix fails it. Gate: - RedeemProviderCall re-proves the abuse pause. ConsumeAttempt linearized it, but that transaction has committed; a pause landing before the dial now invalidates the token. Unlocked read, because the account-control key precedes the operation key this transaction already holds. - SettleProvider requires call_state = 'started', not just a confirmed reservation: a provider id bound to an attempt that never dialed is a ledger claim about a send that did not happen. - The provider message id is normalized to SES's bare form on bind and on compare (NormalizeProviderMessageID). The relay returns it qualified (<id@region.amazonses.com>); SNS feedback carries it bare; the worker and the feedback finalizer must not refuse each other's spelling. - A tenant name that cannot be a header value holds at mint with ses_tenant_unnamed instead of wedging silently in the adapter. Adapter: - X-E2A-Message-ID is derived from the token (a customer operation IS its message id), not trusted from the caller; Envelope.MessageID is gone. - RCPT TO is issued from AuthorizedRecipients(), the canonical envelope the budget priced, never the caller's spelling of it. - A bare CR anywhere in the header section, or a leading continuation line, is refused before redemption (ErrMalformedHeaderSection): a receiver that treats a lone CR as a line break would see a header this walker did not. - Provider headers are emitted in the legacy path's order (configuration set first) so the worker swap is byte-identical for shared headers. - The relay marks a failure after the terminating dot with ErrProviderAcceptanceUnknown; the adapter leaves it unsettled and no classifier calls it permanent. Tests: pause between consume and submit; settle without redeem; bare CR and leading continuation refused with zero sockets; canonical RCPT on the wire; lost 250 unsettled and marked; 4xx unsettled; accepted-but-unsettled reported on the result with a nil error; a positive control proving the socket counter observes a dial; provider id normalization across the three spellings. The socket counter now waits for the accept goroutine. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjfGxvXW6fNKWGFHuo68yX
jiashuoz
added a commit
that referenced
this pull request
Sep 5, 2026
Settlement reports what the provider did, so it is only meaningful for an attempt that opened the socket. The adapter (B5, #998) redeems the token immediately before it dials, and the gate there refuses to settle an attempt whose call_state is not 'started'. These tests settled straight after ConsumeAttempt; route them through a consumeAndRedeem helper so the two slices can land in either order. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjfGxvXW6fNKWGFHuo68yX
Re-review of the previous fix commit found the marker leaking in both directions. The relay's deferred context remap replaced the error wholesale, so a deadline or cancellation after the terminating dot — the likeliest way to lose a 250 — dropped ErrProviderAcceptanceUnknown and classified as a connection outage the worker would re-drive. And the marker was joined to every post-DATA failure, including a coded 554 content rejection, which is the provider's definite answer and must classify permanent. Now the marker survives the remap and is attached only when no reply code came back. Also pins the customer-only guard on the redeem-time pause re-check: the notice telling an account it was paused is sourced from that paused account and must still go out. Reviewer-authored test adopted. And the compare-side normalization of the provider id now has a test that writes a qualified spelling directly and replays bare. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjfGxvXW6fNKWGFHuo68yX
jiashuoz
added a commit
that referenced
this pull request
Sep 5, 2026
* feat(sending): compose ramp with protection gate B4 of the sending-protection plan. The custom-domain ramp and the sending budget answer different questions — "has this domain earned this volume yet?" versus "has this account or the platform exposed SES enough today?" — and this slice makes one transaction answer both, most restrictive winning. One lock order for the ramp. The store previously used three: Reserve took domain → scope → reservation → counter, Confirm took reservation → counter → scope, Release took reservation → counter. Three orders over four keys is a deadlock waiting for traffic, and it becomes unavoidable once the gate composes both ledgers into one transaction, because that transaction already holds highly contended global budget counters when it reaches the ramp. internal/sendramp/tx.go now holds the single order (domain identity → registrable-domain scope → message reservation → UTC day counter) and the pool-owning methods are thin wrappers, so there is no second implementation to drift. Composition, in internal/sendingpolicy/ramp.go: - Probation is now the ramp's answer rather than a stand-in. Shared-relay traffic stays probationary at every plan level and never graduates; a custom domain is probationary until its scope has one qualified day. That classification decides which budget counters the transaction must lock, and the budget counters come BEFORE the ramp keys in the normative order — so it is read unlocked. That is sound only because ramp progress is monotonic: a stale answer can be stale in the strict direction and no other. - The ramp is authorized last, after the budget has been reacquired. A ramp hold therefore arrives with budget units already taken, and those are released before returning the hold — keeping them would charge an account for a send its own domain was not allowed to make. - SettleProvider now moves the ramp: acceptance advances a qualified day, a definite permanent rejection releases the units, and retryable or ambiguous results leave the reservation standing, because a message that might have been delivered must not release capacity. - CancelAttempt releases both ledgers; DeferAttempt still releases only the budget. A rate deferral was not rejected by anyone, and giving back its ramp claim would let the same message re-qualify a stage it already qualified. Disabled mode is genuinely pass-through: no scope row, no counter, and above all no `exempt` write. Writing `exempt` while the ramp is off would permanently grandfather every domain that happened to send during the disabled window, and the phase-3 activation would then find nothing left to ramp. Production ships in exactly this state. A fixture note worth keeping: the ramp ledger is keyed by REGISTRABLE domain, so `ramp-1.example.test` and `ramp-2.example.test` are one scope. The first version of these tests read the hostname key, found empty rows, and would have passed for the wrong reason. Each fixture domain is now its own eTLD+1, and the one test that is about sharing builds two hostnames under a single registrable domain deliberately. Tests: 264 in the package plus the existing ramp suite, green under -race. Covers the 150/213/277 stage caps and their 75/107/139 qualification bars, every probation class, budget-allow/ramp-hold and its mirror, settlement idempotency, permanent-rejection release, defer-versus-cancel, subdomain scope sharing, and a Free account that can qualify stage one but not stage two without losing its progress. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjfGxvXW6fNKWGFHuo68yX * fix(sending): keep the ramp binding under retry, refusal, and rebind Review of the ramp composition found seven ways the stage cap stopped being a cap. All of them come from one shape: the sending budget is keyed by submission ATTEMPT while the ramp reservation is keyed by MESSAGE, and code that treats the two as interchangeable gives away capacity that was already spent. The refund. CancelAttempt released the ramp for any cancelled attempt. Attempt one authorizes and hands 100 recipients to SES, the result is ambiguous so nothing settles and the reservation correctly stands, River allocates attempt two, a suppression cancels it — and the ramp counter goes to zero for mail that is already in flight. It repeats, so the cap became advisory. cancelRamp now asks about the OPERATION rather than the ordinal: if any attempt was ever authorized, only SettleProvider may give those units back. A reservation no attempt has authorized — the shape today's outbound worker produces before this module is involved — is still refundable, which is also where the mirror bug lived: the `released` early return meant a cancel following a deferral never reached the ramp at all. The stranding. A permanent sendramp error (`reservation already released` is reachable by Reserve-after-Cancel on one operation) came back as a hard error, rolling the transaction back with the attempt still `reserved`. Every later execution failed identically, so nothing could ever release its units: 50 recipients pinned on global_all, global_probation, and account_daily until midnight, three of them enough to close probationary sending for the platform. Permanent refusals are now a terminal hold that releases, exactly as the envelope path already answers the same class of loss. The rebind. A domain with an unverified SENDING identity was ramp pass-through AND, because InspectScopeTx never read the column, reported established once its scope had a qualified day — no cap and no probation charge. The wire identity is frozen at acceptance but the agent's registered domain is not: verifying a child subdomain rebinds the account's agents onto it, the child's SES identity stays pending while its DKIM is never published, and the ramp resolves the domain live. An accepted backlog went out uncapped under the parent's frozen From. It now holds, and classifies as probationary. The two legacy states that mean "this domain already earned its volume" — `exempt` and `complete` — are checked first and are untouched. Three smaller ones. Reserve still classified probation as `op.Shared` with a comment deferring to a task this commit is; the early hold therefore never bounded the probation pool, the stored column disagreed with the class every release targets, and each authorization paid a needless release-and-reacquire on the platform's hottest counter rows. The ramp's own source read answered a vanished message with a RETRYABLE hold, and because it runs before the envelope resolution that answers it terminally, the wrong answer won whenever the ramp was armed — a worker snoozing forever instead of failing once. And in the ledger itself: a released-to-confirmed restoration errored instead of no-opping when maintenance had reaped the day's counter (the reservation outlives it by design), while ReleaseTx recorded `released` even when its guarded decrement matched nothing, so a later restoration added back units the counter never returned. Tests: every fix above has a regression test that fails before it. The file also gained the coverage Task 4 Step 2 asks for and did not have — racing workers against one stage cap with the budget counters contended (exactly one cap admitted, no deadlock), cross-midnight re-age of BOTH ledgers in one authorization, settlement arriving days after its attempt, and a FOR UPDATE NOWAIT probe that proves the named domain → scope → reservation → day-counter suborder is actually taken rather than merely documented. Weak assertions were tightened: the probation tests now assert the hold REASON, the ramp-hold test checks all three pools it charged, and the Free-plan test proves progression RESUMES after upgrade rather than merely surviving it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjfGxvXW6fNKWGFHuo68yX * test(sendramp): pin InspectScopeTx classification in its own package InspectScopeTx decides which budget pool a send charges before the ramp lock is taken, but every test of it lived in sendingpolicy, so the per-package coverage gate saw it at 0% and failed the package at 76.6%. Pin each branch where the code lives: missing domain, legacy exempt, stamped complete, unverified identity with an old scope (must stay in probation and ignore the scope), verified with no scope (day zero), a completed scope behind a still-ramping domain row, the day-zero/day-one boundary, and registrable-domain scope lookup with a hostname-keyed decoy. Removing the unverified guard makes the fourth test fail. Package coverage: 76.6% -> 84.9%. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjfGxvXW6fNKWGFHuo68yX * test(sendingpolicy): redeem before settling in the ramp tests Settlement reports what the provider did, so it is only meaningful for an attempt that opened the socket. The adapter (B5, #998) redeems the token immediately before it dials, and the gate there refuses to settle an attempt whose call_state is not 'started'. These tests settled straight after ConsumeAttempt; route them through a consumeAndRedeem helper so the two slices can land in either order. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01AjfGxvXW6fNKWGFHuo68yX --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…-submit # Conflicts: # internal/sendingpolicy/gate.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Slice B5 of the sending abuse prevention plan (Task 5: token-requiring SMTP adapter).
outbound.ProviderSubmitter— the provider seam.SubmitOnce(ctx, auth, Envelope)requires asendingpolicy.ProviderAuthorization, validates the envelope against it, derivesX-E2A-Provider-Attempt,X-SES-TENANT, and the stableX-E2A-Message-IDonly from the token (plusX-SES-CONFIGURATION-SETfrom config), strips every occurrence of those headers from the composed MIME (mixed case, duplicates, folded continuations), redeems the token, then dials. RCPT TO is issued from the token's canonical envelope. Every early exit is I/O-free and leaves the token spendable; an unconfigured relay is refused before redemption so retries do not burn ordinals.ProviderSettlementgainsProviderMessageID, whichSettleProviderbinds to the attempt'ssending_feedback_correlationsrow exactly once, normalized to SES's bare form (NormalizeProviderMessageID) so the worker and the future feedback finalizer agree on one spelling. A different id returnsErrProviderMessageIDConflict. Settlement now also requires the attempt to have actually dialed (call_state = 'started'). A settlement that fails after acceptance is reported onProviderResult.SettlementErr, never as a send error, so callers retry settlement and never resubmit.RedeemProviderCallre-proves the abuse pause immediately before the socket opens; a tenant name that cannot be a header value holds at mint with a visible reason instead of wedging in the adapter.ErrProviderAcceptanceUnknown; no classifier calls it permanent and the seam leaves it unsettled.internal/testutil/testdb— the DB helper extracted into a leaf package.testutilbundles the contract server, which importsoutbound; onceoutboundimportssendingpolicy, any internal test file usingtestutil.TestDBin either package is an import cycle.testutil.TestDBand friends remain as forwarding wrappers; no existing caller changes.What this does NOT do
Sender.SubmitOnceand the raw relay methods remain, unchanged, for the current path and the notification callers.sending_feedback_correlations.provider_message_idUNIQUE (one-attempt-per-id). Follow-up migration.Prod behaviour is unchanged: nothing calls
ProviderSubmitteryet.Review round 1
Two parallel reviews (correctness + adversarial). Findings addressed in
fix(outbound): harden the provider seam after review— each has a named test and was mutation-checked: pause between consume and submit; settle without redeem; provider-id shape mismatch (qualified vs bare); caller-supplied message id; RCPT from the caller's spelling; bare-CR /\r\r\npseudo-separator and leading-continuation header smuggling; racy socket counter with no positive control; ambiguous outcome and accepted-but-unsettled shapes untested; lost-250 unclassifiable; CRLF tenant name mintable via DB edit; header order vs the legacy path.Deferred with rationale above: MAIL FROM binding, UNIQUE provider id index.
Review round 2
Mutation-tested re-review of the fix commit: no blockers, three should-fixes, all addressed in
fix(outbound): keep the acceptance-unknown marker honestwith named tests that fail under mutation — the marker now survives the relay's context remap (deadline/cancel after the dot), is attached only when no reply code came back (a post-DATA 554 stays permanent), and the customer-only guard on the redeem-time pause re-check is pinned by the reviewer's own test (a pause notice to a paused owner must still redeem). The compare-side id normalization also gained a direct test.Notes for B6: a pause caught at redemption surfaces as
ErrAuthorizationInvalid(the nextConsumeAttemptreturnsaccount_paused), so the worker must not emitemail.failedon it; andErrProviderAcceptanceUnknownis the signal to branch on for "maybe sent".Gate for the next slice
Per the plan's B5 row: main worker has not switched; old caller path remains available only inside the package.
🤖 Generated with Claude Code
https://claude.ai/code/session_01AjfGxvXW6fNKWGFHuo68yX